src: support V8 sandbox memory cage in allocators#62237
src: support V8 sandbox memory cage in allocators#62237codebytere wants to merge 1 commit intonodejs:mainfrom
Conversation
nodejs-github-bot
added
c++
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62237 +/- ##
==========================================
- Coverage 89.67% 89.66% -0.02%
==========================================
Files 676 676
Lines 206469 206503 +34
Branches 39537 39543 +6
==========================================
+ Hits 185157 185161 +4
- Misses 13448 13457 +9
- Partials 7864 7885 +21
🚀 New features to boost your workflow:
|
When V8_ENABLE_SANDBOX is enabled, all ArrayBuffer backing stores must be allocated within the V8 memory cage — external pointers cannot be directly wrapped and must be copied into V8-managed memory instead. This commit refactors allocators in node_buffer.cc, node_serdes.cc, and node_trace_events.cc to route allocations through ArrayBuffer::Allocator::NewDefaultAllocator() when the sandbox is enabled, ensuring memory lands inside the cage. In node_serdes.cc, ValueSerializer::Delegate is also extended with ReallocateBufferMemory/FreeBufferMemory overrides so the serializer's internal buffer is cage-allocated from the start.
codebytere
force-pushed
the
sandbox-tweak
branch
from
04bfcd6 to
78e9555
Compare
addaleax
left a comment
addaleax
left a comment
There was a problem hiding this comment.
Reading through the diff ... why doesn't the version of ArrayBuffer::NewBackingStore() that takes a size argument use the default allocator if that's a requirement anyway? Wouldn't it be a bug in V8 if that variant of ArrayBuffer::NewBackingStore() is effectively unusable?
If we indeed need to manually reach out to the default allocator, it would probably be better to create a helper that covers all of these cases and handles the V8_ENABLE_SANDBOX switching in one location instead of those being spread out throughout the codebase
addaleax
left a comment
addaleax
left a comment
There was a problem hiding this comment.
This definitely fixes issues building Node.js with sandbox mode enabled, but it's still a bit unclear how this interacts with fb21829 and why specifically ArrayBuffer::NewBackingStore(isolate, size) doesn't return a backing store that is valid for the given isolate
| size_t size = static_cast<size_t>(value); | ||
| #ifdef V8_ENABLE_SANDBOX | ||
| CHECK_LE(size, kMaxSafeBufferSizeForSandbox); | ||
| CHECK_LE(size, v8::internal::kMaxSafeBufferSizeForSandbox); |
There was a problem hiding this comment.
| CHECK_LE(size, v8::internal::kMaxSafeBufferSizeForSandbox); | |
| CHECK_LE(size, ArrayBuffer::kMaxByteLength); |
We shouldn't access v8::internal unless necessary – ArrayBuffer::kMaxByteLength is defined as v8::internal::kMaxSafeBufferSizeForSandbox in V8_ENABLE_SANDBOX mode
| // (which uses the default IsolateGroup's sandbox) are valid for all | ||
| // isolates. Creating new groups would give each group its own sandbox, | ||
| // causing a mismatch with the allocator. | ||
| if (IsolateGroup::CanCreateNewGroups()) { |
There was a problem hiding this comment.
Should this function even return true if V8_ENABLE_SANDBOX is set?
| void* Reallocate(void* data, size_t old_length, size_t new_length) { | ||
| if (old_length == new_length) return data; | ||
| uint8_t* new_data = reinterpret_cast<uint8_t*>( | ||
| GetAllocator()->AllocateUninitialized(new_length)); |
There was a problem hiding this comment.
This leaks the v8::ArrayBuffer::Allocator* object
There was a problem hiding this comment.
Ah, nvm, I saw that the allocator is defined as static... that's not a real leak then, although it would of course be nice to have proper memory cleanup in place
addaleax
dismissed
their stale review
Memory leak is only real in specific scenarios
4 participants
When
V8_ENABLE_SANDBOXis enabled, all ArrayBuffer backing stores must be allocated within the V8 memory cage — external pointers cannot be directly wrapped and must be copied into V8-managed memory instead. This commit refactors allocators innode_buffer.cc,node_serdes.cc, andnode_trace_events.ccto route allocations throughArrayBuffer::Allocator::NewDefaultAllocator()when the sandbox is enabled, ensuring memory lands inside the cage. Innode_serdes.cc,ValueSerializer::Delegateis also extended withReallocateBufferMemory/FreeBufferMemoryoverrides so the serializer's internal buffer is cage-allocated from the start.Tested by making the following change:
and running with
./configure --ninja --experimental-enable-pointer-compressionThis allows Electron to reduce/remove a patch.